Auto detect and merge lockfile conflicts #3544

sebmck · 2017-05-31T11:54:12Z

Summary

Git merge conflicts are common with yarn.lock. This PR allows you to run yarn to automatically merge the two versions of yarn.lock which will run Yarn and regenerate a new lockfile.

Test plan

Added tests.

cpojer · 2017-05-31T11:56:12Z

src/lockfile/parse.js

@@ -314,9 +314,80 @@ export class Parser {
  }
 }

-export default function(str: string, fileLoc: string = 'lockfile'): Object {
-  str = stripBOM(str);
+const GIT_MERGE_CONFLICT_START = '<<<<<<<';


Drop the "GIT_", other tools will generate similar conflict markers.

arcanis · 2017-05-31T13:23:30Z

src/lockfile/parse.js

+      let variantTwo = [];
+      while (lines.length) {
+        const line = lines.shift();
+        if (line.startsWith(MERGE_CONFLCIT_END)) {


arcanis · 2017-05-31T13:25:37Z

src/lockfile/parse.js

+        if (line === MERGE_CONFLICT_SEP) {
+          break;
+        } else {
+          variantOne.push(line);


can't you just push directly into variants[0]? or at least use let variantOne = variants[0] so that the concat becomes unneeded

arcanis · 2017-05-31T13:25:59Z

src/lockfile/parse.js

+      variants[0] = variants[0].concat(variantOne);
+
+      // get the second variant
+      let variantTwo = [];


same as above

sebmck · 2017-05-31T13:37:32Z

Addressed nits, thanks @arcanis!

arcanis · 2017-05-31T15:25:20Z

@kittens The tests seem to break here, and in a few other place (which all seem related to the resolver .. which is weird since your PR doesn't alter it ôo)

dustinspecker · 2017-05-31T16:16:43Z

src/lockfile/parse.js

+      // get the first variant
+      while (lines.length) {
+        const line = lines.shift();
+        if (line === MERGE_CONFLICT_SEP) {


This feature looks awesome.

Curious if you all are wanting to support git's diff3 merge conflict style (https://git-scm.com/docs/git-merge#_how_conflicts_are_presented). I think this code will break for users using diff3 because the first variant would include both the real variant and the common parent ancestor of both conflicts. I wouldn't mind making a PR in the future for supporting diff3.

Sorry if I'm overlooking something.

sejoker · 2017-05-31T22:34:40Z

src/lockfile/parse.js

+ */
+
+function parseWithConflict(str: string, fileLoc: string): ParseResult {
+  const variants = extractConflictVariants(str);


We assume here that only 1 merge conflict exists. We could handle 2+ conflicts if call parseWithConflict recursively.

It supports multiple conflicts in the same file, but during a same conflict pass. I think it should be good enough for a first iteration :)

@arcanis cool, would be nice to add an extra test for in that case.

sejoker · 2017-05-31T22:36:02Z

__tests__/lockfile.js

@@ -193,3 +193,44 @@ test('Lockfile.getLockfile (sorting)', () => {

  expect(actual).toEqual(expected);
 });
+
+test('parse merge conflicts', () => {


better name -> 'parse single merge conflict'

caesarsol · 2017-06-16T15:54:04Z

@kittens neat! Could you please add git diff3 conflictstyle support?
http://psung.blogspot.it/2011/02/reducing-merge-headaches-git-meets.html?m=1

cpojer · 2017-07-06T14:36:11Z

Rebased, fixed up the tests, made flow more strict. Added support for diff3 by discarding the common ancestor from the parsed result. It seems to me like the information shown is purely for humans to make better assumptions about what to do in case of a conflict, which doesn't apply here. Let me know if that is wrong.

Anything else needed here to get this merged?

BYK

Overall LGTM.

One question: should we make this behavior opt-in via a --merge flag?

BYK · 2017-07-06T14:29:42Z

__tests__/lockfile.js

@@ -193,3 +193,101 @@ test('Lockfile.getLockfile (sorting)', () => {

  expect(actual).toEqual(expected);
 });
+
+test('parse single merge conflict', () => {
+  const file = `


Shall we not put these into a separate file? I can see both ways so just raising the question, not strong opinions.

It's much easier to manage it like this, and we care more about the text input than we care about where it is coming from for the purpose of this test.

BYK · 2017-07-06T14:32:54Z

__tests__/lockfile.js

+
+  const {type, object} = parse(file);
+  expect(type).toEqual('merge');
+  expect(object.a.no).toEqual('yes');


Why not

expect(object).toEqual({ a: { no: 'yes' }, b: { foo: 'bar' }, c: { bar: 'foo' }, d: { yes: 'no' }, });

BYK · 2017-07-06T14:34:13Z

__tests__/lockfile.js

+<<<<<<< HEAD
+b:
+  foo: "bar"
+=======


Why not try adding both, which is most probably what is needed when installing packages?

Ah, it might not be obvious but in this example, the format is actually wrong because yarn lockfiles don't support colons :, which is why it results in a conflict.

BYK · 2017-07-06T14:36:09Z

src/lockfile/parse.js

+
+export function extractConflictVariants(str: string): Array<string> {
+  const variants: Array<Array<string>> = [[], []];
+  const lines = str.split(/\n/g);


Is this safe or should we do [\r\n] instead?

BYK · 2017-07-07T10:54:24Z

src/lockfile/parse.js

+    if (line.startsWith(MERGE_CONFLICT_START)) {
+      // get the first variant
+      while (lines.length) {
+        const line = lines.shift();


Better to not mask the outer variable?

BYK · 2017-07-07T10:56:00Z

src/lockfile/parse.js

+ * Check if a lockfile has merge conflicts.
+ */
+function hasMergeConflicts(str: string): boolean {
+  return str.includes(MERGE_CONFLICT_START);


Wouldn't it be better to check if all 3 markers exist?

cpojer · 2017-07-07T13:07:31Z

I just tried this on Jest jestjs/jest#3906 and it worked flawlessly, this feature is so awesome. @kittens thanks for the idea and getting us 99% there!

**Summary** Follow up to #3544. Currently, `yarn` happily keeps moving if it detects merge conflicts and is able to resolve them in the lockfile. That said it doesn't persist the resolution by saving the lockfile to disk again. This patch ensures writing the lockfile if it is "dirty". This patch also causes `yarn` to throw an error if there are merge conflicts in the file and `--frozen-lockfile` option is true. **Test plan** Existing unit tests. Also try running `yarn install` with the following files: `package.json` ``` { "name": "yarnlock-auto-merge", "version": "1.0.0", "main": "index.js", "author": "Burak Yigit Kaya <byk@fb.com>", "license": "MIT", "dependencies": { "left-pad": "^1.1.3", "right-pad": "^1.0.1" } } ``` `yarn.lock` ``` <<<<<<< HEAD left-pad@^1.1.3: version "1.1.3" resolved "https://registry.yarnpkg.com/left-pad/-/left-pad-1.1.3.tgz#612f61c033f3a9e08e939f1caebeea41b6f3199a" ======= right-pad@^1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/right-pad/-/right-pad-1.0.1.tgz#8ca08c2cbb5b55e74dafa96bf7fd1a27d568c8d0" >>>>>>> right-pad ``` Without the patch, `yarn` won't update the lockfile. With the patch, the lockfile is replaced with the merged version.

) **Summary** Follow up to #3544. Currently, `yarn` happily keeps moving if it detects merge conflicts and is able to resolve them in the lockfile. That said it doesn't persist the resolution by saving the lockfile to disk again. This patch ensures writing the lockfile if it is "dirty". This patch also causes `yarn` to throw an error if there are merge conflicts in the file and `--frozen-lockfile` option is true. **Test plan** Existing unit tests. Also try running `yarn install` with the following files: `package.json` ``` { "name": "yarnlock-auto-merge", "version": "1.0.0", "main": "index.js", "author": "Burak Yigit Kaya <byk@fb.com>", "license": "MIT", "dependencies": { "left-pad": "^1.1.3", "right-pad": "^1.0.1" } } ``` `yarn.lock` ``` <<<<<<< HEAD left-pad@^1.1.3: version "1.1.3" resolved "https://registry.yarnpkg.com/left-pad/-/left-pad-1.1.3.tgz#612f61c033f3a9e08e939f1caebeea41b6f3199a" ======= right-pad@^1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/right-pad/-/right-pad-1.0.1.tgz#8ca08c2cbb5b55e74dafa96bf7fd1a27d568c8d0" >>>>>>> right-pad ``` Without the patch, `yarn` won't update the lockfile. With the patch, the lockfile is replaced with the merged version.

karldanninger · 2017-09-08T15:17:24Z

A+ 👍 👍

wodin · 2017-09-26T21:05:05Z

@cpojer, it's a bad habit to do this:
$ rm -rf packages/*/yarn.lock
Sorry. Pet peeve :) There's no recursion happening there so why use -r? Also it's unlikely you'd need the -f.
But I know of someone at a small ISP who was trying to delete a mailbox and typed "rm -rf /var/spool/mail/" and then pasted the username and pressed enter. When it took longer than expected he suddenly realised there was a space before the username. A lot of mail was lost. Would not have been a problem if he had not used "-rf".
I suspect someone (possibly your distribution) encouraged that habit by aliasing "rm" to "rm -i", but the fix for that is to remove the stupid alias! :) Not to use "-rf" as a matter of course.
Anyway, that's enough of a rant from me. Sorry for the off-topic comment.

raix · 2017-09-27T10:52:38Z

Would be nice to update the # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. in yarn.lock adding a hint to guide the user?
(sorry about the late note)

Vanuan · 2017-09-27T12:07:32Z

What are implications of using yarn install --pure-lock-file wrt resolving conflicts?

BYK · 2017-09-27T12:10:32Z

@Vanuan I expect that to not change the file at all but use the merged version in-memory.

Vanuan · 2017-09-27T12:15:15Z

So how do I resolve conflicts without upgrading packages? Only by hand?

BYK · 2017-09-27T12:17:53Z

@Vanuan I don't really follow that reasoning. yarn install without --pure-lockfile wouldn't (or at least shoudn't) upgrade packages.

Vanuan · 2017-09-27T13:31:56Z

Hm. Somehow I was under impression that it would. I've seen 3 situations where yarn.lock ends up changed:

edit a version in package.json
run yarn add/remove
remove yarn.lock to resolve merge conflicts and run yarn install

The 3rd situation is mitigated by this PR. But it isn't clear whether conflicted packages would be resolved again potentially upgrading the version of unrelated packages.
The 2nd situation might upgrade versions if there are requirements of installed/removed packages changed (and it doesn't conflict with package.json)
But the 1st situation I'm not sure about. Might be the same as 2nd.

Well, it looks like --pure-lock-file only helps if package.json constraints don't match yarn.lock. I.e. when yarn install would change yarn.lock. Probably that situation should be catched by CI:

run yarn install
check if git diff --exit-code fails and faild the build with message "yarn.lock isn't up to date"

In this case --pure-lock-file might not be needed at all.

Vanuan · 2017-09-27T13:42:13Z

The "upgrade" i'm referring to is when yarn install fetches new version that satisfies the version in package.json

yarn install with package@^1.0.0 generates lock file with package@1.0.1
package 1.1.0 is released
yarn install with package@^1.0.0 generates lock file with package@1.1.0

So the question is whether package version gets resolved again in case of merge conflicts.

BYK · 2017-09-29T11:17:04Z

@Vanuan it is quite hard to keep track of what needs to be done under a closed PR. Would you mind summarizing this in a new issue as a feature request or a bug so it is clear to anyone outside of this PR what is asked to be done. This way it would be asier to reason about and work on it.

Vanuan · 2017-09-29T16:05:17Z

Thanks for you patience. This probably doesn't belong here on GitHub.

cpojer · 2017-09-29T16:35:11Z

I'm locking this. The conversation that evolved isn't related to the original issue, if you have concerns about this issue, please send a pull request to fix it. Thanks!

cpojer requested a review from arcanis May 31, 2017 11:55

cpojer reviewed May 31, 2017

View reviewed changes

sebmck force-pushed the lockfile-conflicts branch 3 times, most recently from afffeb6 to 37116f4 Compare May 31, 2017 12:14

AntonNiklasson mentioned this pull request May 31, 2017

Should yarn.lock be treated as a binary file in git? #1776

Closed

arcanis reviewed May 31, 2017

View reviewed changes

sebmck force-pushed the lockfile-conflicts branch from 37116f4 to b0c3929 Compare May 31, 2017 13:37

dustinspecker reviewed May 31, 2017

View reviewed changes

sejoker reviewed May 31, 2017

View reviewed changes

magicmark mentioned this pull request Jun 5, 2017

Selective versions resolutions yarnpkg/rfcs#68

Merged

graingert mentioned this pull request Jun 16, 2017

Deal with lockfile merge conflicts in some way #2155

Closed

cpojer self-assigned this Jul 6, 2017

cpojer force-pushed the lockfile-conflicts branch from bf5d212 to 29b549f Compare July 6, 2017 14:25

cpojer requested review from BYK and arcanis July 6, 2017 14:26

cpojer force-pushed the lockfile-conflicts branch from 29b549f to 1ca53f3 Compare July 6, 2017 14:34

BYK approved these changes Jul 7, 2017

View reviewed changes

Sebastian McKenzie and others added 4 commits July 7, 2017 13:14

auto detect and merge lockfile conflicts

45fe2bf

Skip over common ancestors when using diff3.

2003e8b

Small code cleanups

6eef7da

Nits

2c8a0b2

cpojer force-pushed the lockfile-conflicts branch from 1ca53f3 to 2c8a0b2 Compare July 7, 2017 12:23

cpojer merged commit 3bfa1e3 into master Jul 7, 2017

cpojer deleted the lockfile-conflicts branch July 7, 2017 13:21

gerges-zz mentioned this pull request Jul 30, 2017

Auto-detect and merge lockfile conflicts npm/npm#18007

Closed

luke-john mentioned this pull request Aug 3, 2017

no package-lock.json paypal/glamorous#271

Closed

BYK mentioned this pull request Aug 17, 2017

Fix: Lockfile should be updated when merge conflcits are resolved #4195

Merged

hiimbex mentioned this pull request Sep 11, 2017

Resolving conflicts for package-lock.json npm/npm#18491

Closed

raix mentioned this pull request Sep 27, 2017

What's the intended workflow of the tarball cache? #393

Closed

yarnpkg locked and limited conversation to collaborators Sep 29, 2017

Auto detect and merge lockfile conflicts #3544

Auto detect and merge lockfile conflicts #3544

Conversation

sebmck commented May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmck commented May 31, 2017

arcanis commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarsol commented Jun 16, 2017

cpojer commented Jul 6, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Jul 7, 2017

karldanninger commented Sep 8, 2017 • edited Loading

wodin commented Sep 26, 2017

raix commented Sep 27, 2017

Vanuan commented Sep 27, 2017

BYK commented Sep 27, 2017

Vanuan commented Sep 27, 2017

BYK commented Sep 27, 2017

Vanuan commented Sep 27, 2017 • edited Loading

Vanuan commented Sep 27, 2017 • edited Loading

BYK commented Sep 29, 2017

Vanuan commented Sep 29, 2017

cpojer commented Sep 29, 2017

sebmck commented May 31, 2017 •

edited

Loading

karldanninger commented Sep 8, 2017 •

edited

Loading

Vanuan commented Sep 27, 2017 •

edited

Loading

Vanuan commented Sep 27, 2017 •

edited

Loading